[PATCH] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

[PATCH] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

[Eva Rachel Retuya]

Use IIO_CHAN_INFO_SAMP_FREQ to implement the sampling_frequency attribute
instead of using IIO_DEV_ATTR_SAMP_FREQ() macro.

Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
 drivers/staging/iio/adc/ad7192.c       | 74 +++++++++-------------------------
 include/linux/iio/adc/ad_sigma_delta.h |  1 +
 2 files changed, 21 insertions(+), 54 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index 1cf6b79..154f77f 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -322,57 +322,6 @@ out:
 	return ret;
 }
 
-static ssize_t ad7192_read_frequency(struct device *dev,
-				     struct device_attribute *attr,
-				     char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7192_state *st = iio_priv(indio_dev);
-
-	return sprintf(buf, "%d\n", st->mclk /
-			(st->f_order * 1024 * AD7192_MODE_RATE(st->mode)));
-}
-
-static ssize_t ad7192_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 ad7192_state *st = iio_priv(indio_dev);
-	unsigned long lval;
-	int div, ret;
-
-	ret = kstrtoul(buf, 10, &lval);
-	if (ret)
-		return ret;
-	if (lval == 0)
-		return -EINVAL;
-
-	ret = iio_device_claim_direct_mode(indio_dev);
-	if (ret)
-		return ret;
-
-	div = st->mclk / (lval * st->f_order * 1024);
-	if (div < 1 || div > 1023) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	st->mode &= ~AD7192_MODE_RATE(-1);
-	st->mode |= AD7192_MODE_RATE(div);
-	ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
-
-out:
-	iio_device_release_direct_mode(indio_dev);
-
-	return ret ? ret : len;
-}
-
-static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
-		ad7192_read_frequency,
-		ad7192_write_frequency);
-
 static ssize_t
 ad7192_show_scale_available(struct device *dev,
 			    struct device_attribute *attr, char *buf)
@@ -471,7 +420,6 @@ static IIO_DEVICE_ATTR(ac_excitation_en, S_IRUGO | S_IWUSR,
 		       AD7192_REG_MODE);
 
 static struct attribute *ad7192_attributes[] = {
-	&iio_dev_attr_sampling_frequency.dev_attr.attr,
 	&iio_dev_attr_in_v_m_v_scale_available.dev_attr.attr,
 	&iio_dev_attr_in_voltage_scale_available.dev_attr.attr,
 	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
@@ -484,7 +432,6 @@ static const struct attribute_group ad7192_attribute_group = {
 };
 
 static struct attribute *ad7195_attributes[] = {
-	&iio_dev_attr_sampling_frequency.dev_attr.attr,
 	&iio_dev_attr_in_v_m_v_scale_available.dev_attr.attr,
 	&iio_dev_attr_in_voltage_scale_available.dev_attr.attr,
 	&iio_dev_attr_bridge_switch_en.dev_attr.attr,
@@ -536,6 +483,10 @@ static int ad7192_read_raw(struct iio_dev *indio_dev,
 		if (chan->type == IIO_TEMP)
 			*val -= 273 * ad7192_get_temp_scale(unipolar);
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->mclk /
+			(st->f_order * 1024 * AD7192_MODE_RATE(st->mode));
+		return IIO_VAL_INT;
 	}
 
 	return -EINVAL;
@@ -548,7 +499,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
 			    long mask)
 {
 	struct ad7192_state *st = iio_priv(indio_dev);
-	int ret, i;
+	int ret, i, div;
 	unsigned int tmp;
 
 	ret = iio_device_claim_direct_mode(indio_dev);
@@ -572,10 +523,25 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
 				break;
 			}
 		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (!val)
+			return -EINVAL;
+
+		div = st->mclk / (val * st->f_order * 1024);
+		if (div < 1 || div > 1023) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		st->mode &= ~AD7192_MODE_RATE(-1);
+		st->mode |= AD7192_MODE_RATE(div);
+		ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
+		break;
 	default:
 		ret = -EINVAL;
 	}
 
+out:
 	iio_device_release_direct_mode(indio_dev);
 
 	return ret;
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index e7fdec4..5ba430c 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -136,6 +136,7 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig);
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
 			BIT(IIO_CHAN_INFO_OFFSET), \
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
 		.scan_index = (_si), \
 		.scan_type = { \
 			.sign = 'u', \
-- 
2.7.4

Re: [PATCH] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

[Lars-Peter Clausen]

On 09/30/2016 12:28 PM, Eva Rachel Retuya wrote:
> Use IIO_CHAN_INFO_SAMP_FREQ to implement the sampling_frequency attribute
> instead of using IIO_DEV_ATTR_SAMP_FREQ() macro.
> 
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>

Hi,

Thanks for the patch. There is one tiny bug in the patch, see in the inline
review.

The commit message can also be slightly improved. A commit message should
typically contain why, what and how. For trivial cases some of these might
be omitted but it is always a good idea to include them regardless. In this
case the commit message only includes the what. The how is trivial, so that
is OK. But the why is missing and not necessarily obvious to everybody, e.g.
consider somebody reading the log 5 years from now.

For the why part the commit message could contain that this driver
originally predates the availability of the IIO_CHAN_INFO_SAMP_FREQ
attribute which is why it was not using it and using it has some advantages
like it can be access by in-kernel consumers. And it also reduces the code size.

[...]
> @@ -548,7 +499,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
>  			    long mask)
>  {
>  	struct ad7192_state *st = iio_priv(indio_dev);
> -	int ret, i;
> +	int ret, i, div;
>  	unsigned int tmp;
>  
>  	ret = iio_device_claim_direct_mode(indio_dev);
> @@ -572,10 +523,25 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
>  				break;
>  			}
>  		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (!val)
> +			return -EINVAL;

The direct return here bypasses the iio_device_release_direct_mode() and
will keep the direct mode locked. This sould instead be

if (!val) {
	ret = -EINVAL;
	break;
}

> +
> +		div = st->mclk / (val * st->f_order * 1024);
> +		if (div < 1 || div > 1023) {
> +			ret = -EINVAL;
> +			goto out;

Since this is in a switch statement and we want to jump to the end of the
switch I'd just use break, instead of introducing a new label.

> +		}
> +
> +		st->mode &= ~AD7192_MODE_RATE(-1);
> +		st->mode |= AD7192_MODE_RATE(div);
> +		ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
>  
> +out:
>  	iio_device_release_direct_mode(indio_dev);
>  
>  	return ret;

Re: [PATCH] staging: iio: ad7192: implement IIO_CHAN_INFO_SAMP_FREQ

[Eva Rachel Retuya]

On Fri, Sep 30, 2016 at 02:45:31PM +0200, Lars-Peter Clausen wrote:
> On 09/30/2016 12:28 PM, Eva Rachel Retuya wrote:
> > Use IIO_CHAN_INFO_SAMP_FREQ to implement the sampling_frequency attribute
> > instead of using IIO_DEV_ATTR_SAMP_FREQ() macro.
> > 
> > Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> 
> Hi,
> 
> Thanks for the patch. There is one tiny bug in the patch, see in the inline
> review.
> 
> The commit message can also be slightly improved. A commit message should
> typically contain why, what and how. For trivial cases some of these might
> be omitted but it is always a good idea to include them regardless. In this
> case the commit message only includes the what. The how is trivial, so that
> is OK. But the why is missing and not necessarily obvious to everybody, e.g.
> consider somebody reading the log 5 years from now.
> 
> For the why part the commit message could contain that this driver
> originally predates the availability of the IIO_CHAN_INFO_SAMP_FREQ
> attribute which is why it was not using it and using it has some advantages
> like it can be access by in-kernel consumers. And it also reduces the code size.
> 

Thank you for the comment. I sent the v2 with all your concerns
addressed.

Eva

> [...]
> > @@ -548,7 +499,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
> >  			    long mask)
> >  {
> >  	struct ad7192_state *st = iio_priv(indio_dev);
> > -	int ret, i;
> > +	int ret, i, div;
> >  	unsigned int tmp;
> >  
> >  	ret = iio_device_claim_direct_mode(indio_dev);
> > @@ -572,10 +523,25 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
> >  				break;
> >  			}
> >  		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		if (!val)
> > +			return -EINVAL;
> 
> The direct return here bypasses the iio_device_release_direct_mode() and
> will keep the direct mode locked. This sould instead be
> 
> if (!val) {
> 	ret = -EINVAL;
> 	break;
> }
> 
> > +
> > +		div = st->mclk / (val * st->f_order * 1024);
> > +		if (div < 1 || div > 1023) {
> > +			ret = -EINVAL;
> > +			goto out;
> 
> Since this is in a switch statement and we want to jump to the end of the
> switch I'd just use break, instead of introducing a new label.
> 
> > +		}
> > +
> > +		st->mode &= ~AD7192_MODE_RATE(-1);
> > +		st->mode |= AD7192_MODE_RATE(div);
> > +		ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
> > +		break;
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> >  
> > +out:
> >  	iio_device_release_direct_mode(indio_dev);
> >  
> >  	return ret;