[PATCH] drivers: iio: meter: ade7753: Replace IIO_DEV_ATTR_SAMP_FREQ attribute with IIO_CHAN_INFO_SAMP_FREQ handlers.

[PATCH] drivers: iio: meter: ade7753: Replace IIO_DEV_ATTR_SAMP_FREQ attribute with IIO_CHAN_INFO_SAMP_FREQ handlers.

[Sandhya Bankar]

Moved functionality from IIO_DEV_ATTR_SAMP_FREQ attribute into 
IIO_CHAN_INFO_SAMP_FREQ handlers. Added ade7753_read_raw() and 
ade7753_write_raw() to allow reading/writing the element as well.

Signed-off-by: Sandhya Bankar <bankarsandhya512@gmail.com>
---
 drivers/staging/iio/meter/ade7753.c | 155 +++++++++++++++++++++---------------
 1 file changed, 90 insertions(+), 65 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
index 4b5f05f..a2b4a18 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/drivers/staging/iio/meter/ade7753.c
@@ -98,6 +98,94 @@ static int ade7753_spi_read_reg_16(struct device *dev,
 	return 0;
 }
 
+static int ade7753_read_raw_samp_freq(struct device *dev, int *val)
+{
+	int ret;
+	u16 t;
+
+	ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &t);
+	if (ret)
+		return ret;
+	t = (t >> 11) & 0x3;
+	*val = 27900 / (1 + t);
+
+	return 0;
+}
+
+static int ade7753_write_raw_samp_freq(struct device *dev, int val)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ade7753_state *st = iio_priv(indio_dev);
+	int ret;
+	u16 reg, t;
+
+	if (!val)
+		return -EINVAL;
+
+	t = 27900 / val;
+	if (t > 0)
+		t--;
+
+	if (t > 1)
+		st->us->max_speed_hz = ADE7753_SPI_SLOW;
+	else
+		st->us->max_speed_hz = ADE7753_SPI_FAST;
+
+	ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &reg);
+	if (ret)
+		goto out;
+
+	reg &= ~(3 << 11);
+	reg |= t << 11;
+
+	ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
+
+out:
+	return ret;
+}
+
+static int ade7753_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (val2)
+			return -EINVAL;
+		mutex_lock(&indio_dev->mlock);
+		ret = ade7753_write_raw_samp_freq(&indio_dev->dev, val);
+		mutex_unlock(&indio_dev->mlock);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+static int ade7753_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val,
+		            int *val2,
+			    long mask)
+{
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&indio_dev->mlock);
+		ret = ade7753_read_raw_samp_freq(&indio_dev->dev, val);
+		mutex_unlock(&indio_dev->mlock);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
 static int ade7753_spi_read_reg_24(struct device *dev,
 		u8 reg_address,
 		u32 *val)
@@ -383,82 +471,17 @@ err_ret:
 	return ret;
 }
 
-static ssize_t ade7753_read_frequency(struct device *dev,
-		struct device_attribute *attr,
-		char *buf)
-{
-	int ret;
-	u16 t;
-	int sps;
-
-	ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &t);
-	if (ret)
-		return ret;
-
-	t = (t >> 11) & 0x3;
-	sps = 27900 / (1 + t);
-
-	return sprintf(buf, "%d\n", sps);
-}
-
-static ssize_t ade7753_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 ade7753_state *st = iio_priv(indio_dev);
-	u16 val;
-	int ret;
-	u16 reg, t;
-
-	ret = kstrtou16(buf, 10, &val);
-	if (ret)
-		return ret;
-	if (!val)
-		return -EINVAL;
-
-	mutex_lock(&indio_dev->mlock);
-
-	t = 27900 / val;
-	if (t > 0)
-		t--;
-
-	if (t > 1)
-		st->us->max_speed_hz = ADE7753_SPI_SLOW;
-	else
-		st->us->max_speed_hz = ADE7753_SPI_FAST;
-
-	ret = ade7753_spi_read_reg_16(dev, ADE7753_MODE, &reg);
-	if (ret)
-		goto out;
-
-	reg &= ~(3 << 11);
-	reg |= t << 11;
-
-	ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
-
-out:
-	mutex_unlock(&indio_dev->mlock);
-
-	return ret ? ret : len;
-}
 
 static IIO_DEV_ATTR_TEMP_RAW(ade7753_read_8bit);
 static IIO_CONST_ATTR(in_temp_offset, "-25 C");
 static IIO_CONST_ATTR(in_temp_scale, "0.67 C");
 
-static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
-		ade7753_read_frequency,
-		ade7753_write_frequency);
-
 static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("27900 14000 7000 3500");
 
 static struct attribute *ade7753_attributes[] = {
 	&iio_dev_attr_in_temp_raw.dev_attr.attr,
 	&iio_const_attr_in_temp_offset.dev_attr.attr,
 	&iio_const_attr_in_temp_scale.dev_attr.attr,
-	&iio_dev_attr_sampling_frequency.dev_attr.attr,
 	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
 	&iio_dev_attr_phcal.dev_attr.attr,
 	&iio_dev_attr_cfden.dev_attr.attr,
@@ -496,6 +519,8 @@ static const struct attribute_group ade7753_attribute_group = {
 
 static const struct iio_info ade7753_info = {
 	.attrs = &ade7753_attribute_group,
+	.read_raw = &ade7753_read_raw,
+	.write_raw = &ade7753_write_raw,
 	.driver_module = THIS_MODULE,
 };
 
-- 
1.8.3.1

Re: [PATCH] drivers: iio: meter: ade7753: Replace IIO_DEV_ATTR_SAMP_FREQ attribute with IIO_CHAN_INFO_SAMP_FREQ handlers.

[Greg KH]

On Thu, Sep 29, 2016 at 09:24:17PM +0530, Sandhya Bankar wrote:
> Moved functionality from IIO_DEV_ATTR_SAMP_FREQ attribute into 
> IIO_CHAN_INFO_SAMP_FREQ handlers. Added ade7753_read_raw() and 
> ade7753_write_raw() to allow reading/writing the element as well.

That says what you did, but why would you want to do this?

What benefit is this?  Why do this churn at all?

Changelogs should say why you do something, not just what you did.
Hopefully what you do should be obvious from the diff itself.

thanks,

greg k-h

Re: [PATCH] drivers: iio: meter: ade7753: Replace IIO_DEV_ATTR_SAMP_FREQ attribute with IIO_CHAN_INFO_SAMP_FREQ handlers.

[Sandhya Bankar]

[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --]



On Thursday, September 29, 2016 at 9:29:37 PM UTC+5:30, gregkh wrote:
>
> On Thu, Sep 29, 2016 at 09:24:17PM +0530, Sandhya Bankar wrote: 
> > Moved functionality from IIO_DEV_ATTR_SAMP_FREQ attribute into 
> > IIO_CHAN_INFO_SAMP_FREQ handlers. Added ade7753_read_raw() and 
> > ade7753_write_raw() to allow reading/writing the element as well. 
>
> That says what you did, but why would you want to do this? 
>
> What benefit is this?  Why do this churn at all? 
>
> Changelogs should say why you do something, not just what you did. 
> Hopefully what you do should be obvious from the diff itself. 
>
> thanks, 
>
> greg k-h 
>


Thanks !  got it, I will fix in next revision.



Thanks,
Sandhya  

[-- Attachment #1.2: Type: text/html, Size: 966 bytes --]