[PATCH 1/2] iio: adc: ad7791: remove sample freq sysfs attributes

[PATCH 1/2] iio: adc: ad7791: remove sample freq sysfs attributes

[Alexandru Ardelean]

In the current state, these attributes are broken, because they are
registered already, and the kernel throws a warning.
The first registration happens via the `IIO_CHAN_INFO_SAMP_FREQ` flag from
the `ad_sigma_delta` driver.

In this commit these attrs are removed, and in the following the
IIO_CHAN_INFO_SAMP_FREQ behavior will be implemented, which replaces these
hooks.

This is done to make things a bit easier to review as there is a bit of
overlap in the patch if it's done all at once.

Fixes: a13e831fcaa7 ("staging: iio: ad7192: implement
IIO_CHAN_INFO_SAMP_FREQ")

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/ad7791.c | 49 ------------------------------------------------
 1 file changed, 49 deletions(-)

diff --git a/drivers/iio/adc/ad7791.c b/drivers/iio/adc/ad7791.c
index 70fbf92f9827..03a5f7d6cb0c 100644
--- a/drivers/iio/adc/ad7791.c
+++ b/drivers/iio/adc/ad7791.c
@@ -244,58 +244,9 @@ static int ad7791_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-static const char * const ad7791_sample_freq_avail[] = {
-	[AD7791_FILTER_RATE_120] = "120",
-	[AD7791_FILTER_RATE_100] = "100",
-	[AD7791_FILTER_RATE_33_3] = "33.3",
-	[AD7791_FILTER_RATE_20] = "20",
-	[AD7791_FILTER_RATE_16_6] = "16.6",
-	[AD7791_FILTER_RATE_16_7] = "16.7",
-	[AD7791_FILTER_RATE_13_3] = "13.3",
-	[AD7791_FILTER_RATE_9_5] = "9.5",
-};
-
-static ssize_t ad7791_read_frequency(struct device *dev,
-	struct device_attribute *attr, char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7791_state *st = iio_priv(indio_dev);
-	unsigned int rate = st->filter & AD7791_FILTER_RATE_MASK;
-
-	return sprintf(buf, "%s\n", ad7791_sample_freq_avail[rate]);
-}
-
-static ssize_t ad7791_write_frequency(struct device *dev,
-	struct device_attribute *attr, const char *buf, size_t len)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7791_state *st = iio_priv(indio_dev);
-	int i, ret;
-
-	i = sysfs_match_string(ad7791_sample_freq_avail, buf);
-	if (i < 0)
-		return i;
-
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
-	st->filter &= ~AD7791_FILTER_RATE_MASK;
-	st->filter |= i;
-	ad_sd_write_reg(&st->sd, AD7791_REG_FILTER, sizeof(st->filter),
-			st->filter);
-	iio_device_release_direct_mode(indio_dev);
-
-	return len;
-}
-
-static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
-		ad7791_read_frequency,
-		ad7791_write_frequency);
-
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("120 100 33.3 20 16.7 16.6 13.3 9.5");
 
 static struct attribute *ad7791_attributes[] = {
-	&iio_dev_attr_sampling_frequency.dev_attr.attr,
 	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
 	NULL
 };
-- 
2.14.1

[PATCH 2/2] iio: adc: ad7791: implement IIO_CHAN_INFO_SAMP_FREQ

[Alexandru Ardelean]

Now that the old read/write frequency sysfs attrs have been removed, we
have a clean slate to implement IIO_CHAN_INFO_SAMP_FREQ.

This driver also pre-dates IIO_CHAN_INFO_SAMP_FREQ, and this change
implements this behavior.

The `ad7791_write_raw` would have overlapped quite a bit with the old
read/write frequency functions, making things a bit harder to follow.

Fixes: a13e831fcaa7 ("staging: iio: ad7192: implement
IIO_CHAN_INFO_SAMP_FREQ")

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/ad7791.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/iio/adc/ad7791.c b/drivers/iio/adc/ad7791.c
index 03a5f7d6cb0c..a9ff0695ddf7 100644
--- a/drivers/iio/adc/ad7791.c
+++ b/drivers/iio/adc/ad7791.c
@@ -153,6 +153,17 @@ struct ad7791_state {
 	const struct ad7791_chip_info *info;
 };
 
+static const int ad7791_sample_freq_avail[8][2] = {
+	[AD7791_FILTER_RATE_120] =  { 120, 0 },
+	[AD7791_FILTER_RATE_100] =  { 100, 0 },
+	[AD7791_FILTER_RATE_33_3] = { 33,  300000 },
+	[AD7791_FILTER_RATE_20] =   { 20,  0 },
+	[AD7791_FILTER_RATE_16_6] = { 16,  600000 },
+	[AD7791_FILTER_RATE_16_7] = { 16,  700000 },
+	[AD7791_FILTER_RATE_13_3] = { 13,  300000 },
+	[AD7791_FILTER_RATE_9_5] =  { 9,   500000 },
+};
+
 static struct ad7791_state *ad_sigma_delta_to_ad7791(struct ad_sigma_delta *sd)
 {
 	return container_of(sd, struct ad7791_state, sd);
@@ -202,6 +213,7 @@ static int ad7791_read_raw(struct iio_dev *indio_dev,
 {
 	struct ad7791_state *st = iio_priv(indio_dev);
 	bool unipolar = !!(st->mode & AD7791_MODE_UNIPOLAR);
+	unsigned int rate;
 
 	switch (info) {
 	case IIO_CHAN_INFO_RAW:
@@ -239,11 +251,53 @@ static int ad7791_read_raw(struct iio_dev *indio_dev,
 			*val2 = chan->scan_type.realbits - 1;
 
 		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		rate = st->filter & AD7791_FILTER_RATE_MASK;
+		*val = ad7791_sample_freq_avail[rate][0];
+		*val2 = ad7791_sample_freq_avail[rate][1];
+		return IIO_VAL_INT_PLUS_MICRO;
 	}
 
 	return -EINVAL;
 }
 
+static int ad7791_write_raw(struct iio_dev *indio_dev,
+	struct iio_chan_spec const *chan, int val, int val2, long mask)
+{
+	struct ad7791_state *st = iio_priv(indio_dev);
+	int ret, i;
+
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		for (i = 0; i < ARRAY_SIZE(ad7791_sample_freq_avail); i++) {
+			if (ad7791_sample_freq_avail[i][0] == val &&
+			    ad7791_sample_freq_avail[i][1] == val2)
+				break;
+		}
+
+		if (i == ARRAY_SIZE(ad7791_sample_freq_avail)) {
+			ret = -EINVAL;
+			break;
+		}
+
+		st->filter &= ~AD7791_FILTER_RATE_MASK;
+		st->filter |= i;
+		ad_sd_write_reg(&st->sd, AD7791_REG_FILTER,
+				sizeof(st->filter),
+				st->filter);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	iio_device_release_direct_mode(indio_dev);
+	return ret;
+}
+
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("120 100 33.3 20 16.7 16.6 13.3 9.5");
 
 static struct attribute *ad7791_attributes[] = {
@@ -257,12 +311,14 @@ static const struct attribute_group ad7791_attribute_group = {
 
 static const struct iio_info ad7791_info = {
 	.read_raw = &ad7791_read_raw,
+	.write_raw = &ad7791_write_raw,
 	.attrs = &ad7791_attribute_group,
 	.validate_trigger = ad_sd_validate_trigger,
 };
 
 static const struct iio_info ad7791_no_filter_info = {
 	.read_raw = &ad7791_read_raw,
+	.write_raw = &ad7791_write_raw,
 	.validate_trigger = ad_sd_validate_trigger,
 };
 
-- 
2.14.1

Re: [PATCH 2/2] iio: adc: ad7791: implement IIO_CHAN_INFO_SAMP_FREQ

[Lars-Peter Clausen]

On 03/12/2018 01:06 PM, Alexandru Ardelean wrote:
[...]
> +		for (i = 0; i < ARRAY_SIZE(ad7791_sample_freq_avail); i++) {
> +			if (ad7791_sample_freq_avail[i][0] == val &&
> +			    ad7791_sample_freq_avail[i][1] == val2)
> +				break;
> +		}

Unrelated to this patch. But we should really add a helper function for
looking up entries in these val,val2 arrays.

[...]

Re: [PATCH 2/2] iio: adc: ad7791: implement IIO_CHAN_INFO_SAMP_FREQ

[Jonathan Cameron]

On Mon, 12 Mar 2018 14:06:54 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> Now that the old read/write frequency sysfs attrs have been removed, we
> have a clean slate to implement IIO_CHAN_INFO_SAMP_FREQ.
> 
> This driver also pre-dates IIO_CHAN_INFO_SAMP_FREQ, and this change
> implements this behavior.
> 
> The `ad7791_write_raw` would have overlapped quite a bit with the old
> read/write frequency functions, making things a bit harder to follow.
> 
> Fixes: a13e831fcaa7 ("staging: iio: ad7192: implement
> IIO_CHAN_INFO_SAMP_FREQ")
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied to the togreg branch of iio.git and marked for stable.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7791.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7791.c b/drivers/iio/adc/ad7791.c
> index 03a5f7d6cb0c..a9ff0695ddf7 100644
> --- a/drivers/iio/adc/ad7791.c
> +++ b/drivers/iio/adc/ad7791.c
> @@ -153,6 +153,17 @@ struct ad7791_state {
>  	const struct ad7791_chip_info *info;
>  };
>  
> +static const int ad7791_sample_freq_avail[8][2] = {
> +	[AD7791_FILTER_RATE_120] =  { 120, 0 },
> +	[AD7791_FILTER_RATE_100] =  { 100, 0 },
> +	[AD7791_FILTER_RATE_33_3] = { 33,  300000 },
> +	[AD7791_FILTER_RATE_20] =   { 20,  0 },
> +	[AD7791_FILTER_RATE_16_6] = { 16,  600000 },
> +	[AD7791_FILTER_RATE_16_7] = { 16,  700000 },
> +	[AD7791_FILTER_RATE_13_3] = { 13,  300000 },
> +	[AD7791_FILTER_RATE_9_5] =  { 9,   500000 },
> +};
> +
>  static struct ad7791_state *ad_sigma_delta_to_ad7791(struct ad_sigma_delta *sd)
>  {
>  	return container_of(sd, struct ad7791_state, sd);
> @@ -202,6 +213,7 @@ static int ad7791_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct ad7791_state *st = iio_priv(indio_dev);
>  	bool unipolar = !!(st->mode & AD7791_MODE_UNIPOLAR);
> +	unsigned int rate;
>  
>  	switch (info) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -239,11 +251,53 @@ static int ad7791_read_raw(struct iio_dev *indio_dev,
>  			*val2 = chan->scan_type.realbits - 1;
>  
>  		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		rate = st->filter & AD7791_FILTER_RATE_MASK;
> +		*val = ad7791_sample_freq_avail[rate][0];
> +		*val2 = ad7791_sample_freq_avail[rate][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	}
>  
>  	return -EINVAL;
>  }
>  
> +static int ad7791_write_raw(struct iio_dev *indio_dev,
> +	struct iio_chan_spec const *chan, int val, int val2, long mask)
> +{
> +	struct ad7791_state *st = iio_priv(indio_dev);
> +	int ret, i;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		for (i = 0; i < ARRAY_SIZE(ad7791_sample_freq_avail); i++) {
> +			if (ad7791_sample_freq_avail[i][0] == val &&
> +			    ad7791_sample_freq_avail[i][1] == val2)
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(ad7791_sample_freq_avail)) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		st->filter &= ~AD7791_FILTER_RATE_MASK;
> +		st->filter |= i;
> +		ad_sd_write_reg(&st->sd, AD7791_REG_FILTER,
> +				sizeof(st->filter),
> +				st->filter);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	iio_device_release_direct_mode(indio_dev);
> +	return ret;
> +}
> +
>  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("120 100 33.3 20 16.7 16.6 13.3 9.5");
>  
>  static struct attribute *ad7791_attributes[] = {
> @@ -257,12 +311,14 @@ static const struct attribute_group ad7791_attribute_group = {
>  
>  static const struct iio_info ad7791_info = {
>  	.read_raw = &ad7791_read_raw,
> +	.write_raw = &ad7791_write_raw,
>  	.attrs = &ad7791_attribute_group,
>  	.validate_trigger = ad_sd_validate_trigger,
>  };
>  
>  static const struct iio_info ad7791_no_filter_info = {
>  	.read_raw = &ad7791_read_raw,
> +	.write_raw = &ad7791_write_raw,
>  	.validate_trigger = ad_sd_validate_trigger,
>  };
>  

Re: [PATCH 1/2] iio: adc: ad7791: remove sample freq sysfs attributes

[Jonathan Cameron]

On Mon, 12 Mar 2018 14:06:53 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> In the current state, these attributes are broken, because they are
> registered already, and the kernel throws a warning.
> The first registration happens via the `IIO_CHAN_INFO_SAMP_FREQ` flag from
> the `ad_sigma_delta` driver.
> 
> In this commit these attrs are removed, and in the following the
> IIO_CHAN_INFO_SAMP_FREQ behavior will be implemented, which replaces these
> hooks.
> 
> This is done to make things a bit easier to review as there is a bit of
> overlap in the patch if it's done all at once.
> 
> Fixes: a13e831fcaa7 ("staging: iio: ad7192: implement
> IIO_CHAN_INFO_SAMP_FREQ")
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
I'm going to apply any more of this type via the slow route of the next
merge windows.  I'll mark them for stable though so they make it back
to older to trees after the merge window.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ad7791.c | 49 ------------------------------------------------
>  1 file changed, 49 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7791.c b/drivers/iio/adc/ad7791.c
> index 70fbf92f9827..03a5f7d6cb0c 100644
> --- a/drivers/iio/adc/ad7791.c
> +++ b/drivers/iio/adc/ad7791.c
> @@ -244,58 +244,9 @@ static int ad7791_read_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> -static const char * const ad7791_sample_freq_avail[] = {
> -	[AD7791_FILTER_RATE_120] = "120",
> -	[AD7791_FILTER_RATE_100] = "100",
> -	[AD7791_FILTER_RATE_33_3] = "33.3",
> -	[AD7791_FILTER_RATE_20] = "20",
> -	[AD7791_FILTER_RATE_16_6] = "16.6",
> -	[AD7791_FILTER_RATE_16_7] = "16.7",
> -	[AD7791_FILTER_RATE_13_3] = "13.3",
> -	[AD7791_FILTER_RATE_9_5] = "9.5",
> -};
> -
> -static ssize_t ad7791_read_frequency(struct device *dev,
> -	struct device_attribute *attr, char *buf)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct ad7791_state *st = iio_priv(indio_dev);
> -	unsigned int rate = st->filter & AD7791_FILTER_RATE_MASK;
> -
> -	return sprintf(buf, "%s\n", ad7791_sample_freq_avail[rate]);
> -}
> -
> -static ssize_t ad7791_write_frequency(struct device *dev,
> -	struct device_attribute *attr, const char *buf, size_t len)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct ad7791_state *st = iio_priv(indio_dev);
> -	int i, ret;
> -
> -	i = sysfs_match_string(ad7791_sample_freq_avail, buf);
> -	if (i < 0)
> -		return i;
> -
> -	ret = iio_device_claim_direct_mode(indio_dev);
> -	if (ret)
> -		return ret;
> -	st->filter &= ~AD7791_FILTER_RATE_MASK;
> -	st->filter |= i;
> -	ad_sd_write_reg(&st->sd, AD7791_REG_FILTER, sizeof(st->filter),
> -			st->filter);
> -	iio_device_release_direct_mode(indio_dev);
> -
> -	return len;
> -}
> -
> -static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
> -		ad7791_read_frequency,
> -		ad7791_write_frequency);
> -
>  static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("120 100 33.3 20 16.7 16.6 13.3 9.5");
>  
>  static struct attribute *ad7791_attributes[] = {
> -	&iio_dev_attr_sampling_frequency.dev_attr.attr,
>  	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
>  	NULL
>  };