[PATCH] staging: iio: ad7606: implement IIO_CHAN_INFO_OVERSAMPLING_RATIO

[PATCH] staging: iio: ad7606: implement IIO_CHAN_INFO_OVERSAMPLING_RATIO

[Eva Rachel Retuya]

This driver predates the availability of IIO_CHAN_INFO_OVERSAMPLING_RATIO
attribute wherein usage has some advantages like it can be accessed by
in-kernel consumers as well as reduces the code size.

Therefore, use IIO_CHAN_INFO_OVERSAMPLING_RATIO to implement the
oversampling_ratio attribute instead of using IIO_DEVICE_ATTR() macro.

Move code from the functions associated with IIO_DEVICE_ATTR() into
the read_raw hook as well as add the write_raw hook with both masks set
to IIO_CHAN_INFO_OVERSAMPLING_RATIO.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
 drivers/staging/iio/adc/ad7606_core.c | 67 ++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7606_core.c b/drivers/staging/iio/adc/ad7606_core.c
index f79ee61..759f31d 100644
--- a/drivers/staging/iio/adc/ad7606_core.c
+++ b/drivers/staging/iio/adc/ad7606_core.c
@@ -103,6 +103,9 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 		*val = st->range * 2;
 		*val2 = st->chip_info->channels[0].scan_type.realbits;
 		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = st->oversampling;
+		return IIO_VAL_INT;
 	}
 	return -EINVAL;
 }
@@ -145,16 +148,6 @@ static IIO_DEVICE_ATTR(in_voltage_range, S_IRUGO | S_IWUSR,
 		       ad7606_show_range, ad7606_store_range, 0);
 static IIO_CONST_ATTR(in_voltage_range_available, "5000 10000");
 
-static ssize_t ad7606_show_oversampling_ratio(struct device *dev,
-					      struct device_attribute *attr,
-					      char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	return sprintf(buf, "%u\n", st->oversampling);
-}
-
 static int ad7606_oversampling_get_index(unsigned int val)
 {
 	unsigned char supported[] = {0, 2, 4, 8, 16, 32, 64};
@@ -167,44 +160,43 @@ static int ad7606_oversampling_get_index(unsigned int val)
 	return -EINVAL;
 }
 
-static ssize_t ad7606_store_oversampling_ratio(struct device *dev,
-					       struct device_attribute *attr,
-					       const char *buf, size_t count)
+static int ad7606_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long mask)
 {
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7606_state *st = iio_priv(indio_dev);
-	unsigned long lval;
 	int ret;
 
-	ret = kstrtoul(buf, 10, &lval);
-	if (ret)
-		return ret;
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		if (val2)
+			return -EINVAL;
+		ret = ad7606_oversampling_get_index(val);
+		if (ret < 0) {
+			dev_err(dev, "oversampling %lu is not supported\n",
+				val);
+			return ret;
+		}
 
-	ret = ad7606_oversampling_get_index(lval);
-	if (ret < 0) {
-		dev_err(dev, "oversampling %lu is not supported\n", lval);
-		return ret;
+		mutex_lock(&indio_dev->mlock);
+		gpio_set_value(st->pdata->gpio_os0, (ret >> 0) & 1);
+		gpio_set_value(st->pdata->gpio_os1, (ret >> 1) & 1);
+		gpio_set_value(st->pdata->gpio_os1, (ret >> 2) & 1);
+		st->oversampling = val;
+		mutex_unlock(&indio_dev->mlock);
+		return 0;
+	default:
+		return -EINVAL;
 	}
-
-	mutex_lock(&indio_dev->mlock);
-	gpio_set_value(st->pdata->gpio_os0, (ret >> 0) & 1);
-	gpio_set_value(st->pdata->gpio_os1, (ret >> 1) & 1);
-	gpio_set_value(st->pdata->gpio_os1, (ret >> 2) & 1);
-	st->oversampling = lval;
-	mutex_unlock(&indio_dev->mlock);
-
-	return count;
 }
 
-static IIO_DEVICE_ATTR(oversampling_ratio, S_IRUGO | S_IWUSR,
-		       ad7606_show_oversampling_ratio,
-		       ad7606_store_oversampling_ratio, 0);
 static IIO_CONST_ATTR(oversampling_ratio_available, "0 2 4 8 16 32 64");
 
 static struct attribute *ad7606_attributes_os_and_range[] = {
 	&iio_dev_attr_in_voltage_range.dev_attr.attr,
 	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
-	&iio_dev_attr_oversampling_ratio.dev_attr.attr,
 	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
 	NULL,
 };
@@ -214,7 +206,6 @@ static const struct attribute_group ad7606_attribute_group_os_and_range = {
 };
 
 static struct attribute *ad7606_attributes_os[] = {
-	&iio_dev_attr_oversampling_ratio.dev_attr.attr,
 	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
 	NULL,
 };
@@ -241,6 +232,8 @@ static const struct attribute_group ad7606_attribute_group_range = {
 		.address = num,					\
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),\
+		.info_mask_shared_by_all =			\
+			BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
 		.scan_index = num,				\
 		.scan_type = {					\
 			.sign = 's',				\
@@ -429,12 +422,14 @@ static const struct iio_info ad7606_info_no_os_or_range = {
 static const struct iio_info ad7606_info_os_and_range = {
 	.driver_module = THIS_MODULE,
 	.read_raw = &ad7606_read_raw,
+	.write_raw = &ad7606_write_raw,
 	.attrs = &ad7606_attribute_group_os_and_range,
 };
 
 static const struct iio_info ad7606_info_os = {
 	.driver_module = THIS_MODULE,
 	.read_raw = &ad7606_read_raw,
+	.write_raw = &ad7606_write_raw,
 	.attrs = &ad7606_attribute_group_os,
 };
 
-- 
2.7.4

Re: [PATCH] staging: iio: ad7606: implement IIO_CHAN_INFO_OVERSAMPLING_RATIO

[Lars-Peter Clausen]

On 10/06/2016 04:42 PM, Eva Rachel Retuya wrote:
> This driver predates the availability of IIO_CHAN_INFO_OVERSAMPLING_RATIO
> attribute wherein usage has some advantages like it can be accessed by
> in-kernel consumers as well as reduces the code size.
> 
> Therefore, use IIO_CHAN_INFO_OVERSAMPLING_RATIO to implement the
> oversampling_ratio attribute instead of using IIO_DEVICE_ATTR() macro.
> 
> Move code from the functions associated with IIO_DEVICE_ATTR() into
> the read_raw hook as well as add the write_raw hook with both masks set
> to IIO_CHAN_INFO_OVERSAMPLING_RATIO.
> 
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>

Looks good, thanks.

Acked-by: Lars-Peter Clausen <lars@metafoo.de>

I did some cleanup on this driver last week (not yet submitted), this was
the last bit I hadn't gotten to yet that needs to be done before the driver
can be moved out of staging.

Re: [PATCH] staging: iio: ad7606: implement IIO_CHAN_INFO_OVERSAMPLING_RATIO

[Eva Rachel Retuya]

On Thu, Oct 06, 2016 at 05:43:36PM +0200, Lars-Peter Clausen wrote:
> On 10/06/2016 04:42 PM, Eva Rachel Retuya wrote:
> > This driver predates the availability of IIO_CHAN_INFO_OVERSAMPLING_RATIO
> > attribute wherein usage has some advantages like it can be accessed by
> > in-kernel consumers as well as reduces the code size.
> > 
> > Therefore, use IIO_CHAN_INFO_OVERSAMPLING_RATIO to implement the
> > oversampling_ratio attribute instead of using IIO_DEVICE_ATTR() macro.
> > 
> > Move code from the functions associated with IIO_DEVICE_ATTR() into
> > the read_raw hook as well as add the write_raw hook with both masks set
> > to IIO_CHAN_INFO_OVERSAMPLING_RATIO.
> > 
> > Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> 
> Looks good, thanks.
> 
> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
> 

Thank you for the feedback!

> I did some cleanup on this driver last week (not yet submitted), this was
> the last bit I hadn't gotten to yet that needs to be done before the driver
> can be moved out of staging.

Anything else about this driver that needs to be addressed? I'll be glad
to help submit patches for those (and in the process learn something new).

By the way, this particular code took my attention (from write_raw of
this patch):

		gpio_set_value(st->pdata->gpio_os0, (ret >> 0) & 1);
		gpio_set_value(st->pdata->gpio_os1, (ret >> 1) & 1);
		gpio_set_value(st->pdata->gpio_os1, (ret >> 2) & 1);

The last line, st->pdata->gpio_os1, is that really intended or is it a
typo? It could have been st->pdata->gpio_os2. I checked the datasheet
and definition, looks like a typo to me. Just checking with everyone for
confirmation.

Thanks,
Eva

Re: [PATCH] staging: iio: ad7606: implement IIO_CHAN_INFO_OVERSAMPLING_RATIO

[Lars-Peter Clausen]

On 10/07/2016 08:02 AM, Eva Rachel Retuya wrote:
> On Thu, Oct 06, 2016 at 05:43:36PM +0200, Lars-Peter Clausen wrote:
>> On 10/06/2016 04:42 PM, Eva Rachel Retuya wrote:
>>> This driver predates the availability of IIO_CHAN_INFO_OVERSAMPLING_RATIO
>>> attribute wherein usage has some advantages like it can be accessed by
>>> in-kernel consumers as well as reduces the code size.
>>>
>>> Therefore, use IIO_CHAN_INFO_OVERSAMPLING_RATIO to implement the
>>> oversampling_ratio attribute instead of using IIO_DEVICE_ATTR() macro.
>>>
>>> Move code from the functions associated with IIO_DEVICE_ATTR() into
>>> the read_raw hook as well as add the write_raw hook with both masks set
>>> to IIO_CHAN_INFO_OVERSAMPLING_RATIO.
>>>
>>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>>
>> Looks good, thanks.
>>
>> Acked-by: Lars-Peter Clausen <lars@metafoo.de>
>>
> 
> Thank you for the feedback!
> 
>> I did some cleanup on this driver last week (not yet submitted), this was
>> the last bit I hadn't gotten to yet that needs to be done before the driver
>> can be moved out of staging.
> 
> Anything else about this driver that needs to be addressed? I'll be glad
> to help submit patches for those (and in the process learn something new).

One thing I haven't addressed yet is the regulator handling. Currently the
driver ignores all errors regulator_get(). This is incorrect and among other
things breaks probe deferral (EPROBE_DEFER). The correct behavior is to
propagate the error to the upper layers so they can handle it accordingly.
Another reason to do this is that the device can't work without power
enabled, so it needs a regulator connected. If the specific design uses a
static always-on regulator and does not explicitly specify it
regulator_get() will return the dummy regulator.

Another thing regarding regulator handling is that the names passed to
regulator_get() should match the name of the supply as specified in the
device datasheet. In this case the supply is called AVcc, while the driver
passes just "vcc". Since it is custom to use the lower-caps version of the
datasheet name this should be replaced with "avcc".

If you are interested in this type of issue I think you'll be able to find a
few more in the staging IIO tree that also need to be addressed. And
potentially even in the regular IIO tree. The following cocci snippet should
be able to help you identify potential candidates (it's not very
sophisticated, so there will be false positives and false negatives).

@r1@
expression reg;
position p;
@@
reg = \(devm_regulator_get@p\|regulator_get@p\)(...);
if (IS_ERR(reg)) {
	...
	PTR_ERR(reg)
	...
}

@@
position p != r1.p;
@@
*\(devm_regulator_get@p\|regulator_get@p\)(...)


I've pushed my changes to ad7606 driver for reference to
https://github.com/analogdevicesinc/linux/tree/iio-ad7605

I still need to test them with real hardware before I'll submit them and
this has to wait until after ELCE next week.

> 
> By the way, this particular code took my attention (from write_raw of
> this patch):
> 
> 		gpio_set_value(st->pdata->gpio_os0, (ret >> 0) & 1);
> 		gpio_set_value(st->pdata->gpio_os1, (ret >> 1) & 1);
> 		gpio_set_value(st->pdata->gpio_os1, (ret >> 2) & 1);
> 
> The last line, st->pdata->gpio_os1, is that really intended or is it a
> typo? It could have been st->pdata->gpio_os2. I checked the datasheet
> and definition, looks like a typo to me. Just checking with everyone for
> confirmation.

Nice catch, that's most certainly a bug. I didn't even notice this when
refactoring the code. Please send a fix.