[PATCH 0/3] stagging:iio:meter: Add essential IIO API structures for ADE7854

[PATCH 0/3] stagging:iio:meter: Add essential IIO API structures for ADE7854

[Rodrigo Siqueira]

This patchset aims to update ADE7854 by adding the required IIO API
components. The first patch adds the iio_chan_spec for handling seven
different registers (all of them with a similar behavior). The second
patch appends the read_raw function defined by the IIO API. Finally, the
third patch adds the write_raw function and remove the attributes used
for handling the seven registers. This patchset has the contribution of
John Syne, which was responsible for mapping the correct ABI name per
element in the ADE7854; additionally, John provided codes that helped to
shape these patches.

Rodrigo Siqueira (3):
  stagging:iio:meter: Add iio_chan_spec
  stagging:iio:meter: Add ade7854_read_raw function
  stagging:iio:meter: Add ade7854_write_raw function

 drivers/staging/iio/meter/ade7854.c | 129 ++++++++++++++++++++--------
 1 file changed, 94 insertions(+), 35 deletions(-)

-- 
2.17.0

[PATCH 1/3] stagging:iio:meter: Add iio_chan_spec

[Rodrigo Siqueira]

This patch adds iio_chan_spec struct. Additionally, the channel adds the
support for handling AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and
CVGAIN.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/meter/ade7854.c | 42 +++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index 029c3bf42d4d..2fbb2570ba54 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -22,6 +22,48 @@
 #include "meter.h"
 #include "ade7854.h"
 
+#define PHASEA "phaseA"
+#define PHASEB "phaseB"
+#define PHASEC "phaseC"
+#define NEUTRAL "neutral"
+
+#define ADE7854_CHANNEL(_type, _name, _mask, _reg) {			\
+	.type = _type,							\
+	.indexed = 1,							\
+	.channel = 0,							\
+	.extend_name = _name,						\
+	.info_mask_separate = _mask,					\
+	.address = _reg,						\
+	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = -1,						\
+}
+
+static const struct iio_chan_spec ade7854_channels[] = {
+	/* Current */
+	ADE7854_CHANNEL(IIO_CURRENT, PHASEA,
+			BIT(IIO_CHAN_INFO_SCALE),
+			ADE7854_AIGAIN),
+	ADE7854_CHANNEL(IIO_CURRENT, PHASEB,
+			BIT(IIO_CHAN_INFO_SCALE),
+			ADE7854_BIGAIN),
+	ADE7854_CHANNEL(IIO_CURRENT, PHASEC,
+			BIT(IIO_CHAN_INFO_SCALE),
+			ADE7854_CIGAIN),
+	ADE7854_CHANNEL(IIO_CURRENT, NEUTRAL,
+			BIT(IIO_CHAN_INFO_SCALE),
+			ADE7854_NIGAIN),
+	/* Voltage */
+	ADE7854_CHANNEL(IIO_VOLTAGE, PHASEA,
+			BIT(IIO_CHAN_INFO_SCALE),
+			ADE7854_AVGAIN),
+	ADE7854_CHANNEL(IIO_VOLTAGE, PHASEB,
+			BIT(IIO_CHAN_INFO_SCALE),
+			ADE7854_BVGAIN),
+	ADE7854_CHANNEL(IIO_VOLTAGE, PHASEC,
+			BIT(IIO_CHAN_INFO_SCALE),
+			ADE7854_CVGAIN),
+};
+
 static ssize_t ade7854_read_8bit(struct device *dev,
 				 struct device_attribute *attr,
 				 char *buf)
-- 
2.17.0

Re: [PATCH 1/3] stagging:iio:meter: Add iio_chan_spec

[Jonathan Cameron]

On Sat, 21 Apr 2018 08:55:08 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> This patch adds iio_chan_spec struct. Additionally, the channel adds the
> support for handling AIGAIN, BIGAIN, CIGAIN, NIGAIN, AVGAIN, BVGAIN, and
> CVGAIN.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> ---
>  drivers/staging/iio/meter/ade7854.c | 42 +++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> index 029c3bf42d4d..2fbb2570ba54 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -22,6 +22,48 @@
>  #include "meter.h"
>  #include "ade7854.h"
>  
> +#define PHASEA "phaseA"
> +#define PHASEB "phaseB"
> +#define PHASEC "phaseC"
> +#define NEUTRAL "neutral"
> +
> +#define ADE7854_CHANNEL(_type, _name, _mask, _reg) {			\
> +	.type = _type,							\
> +	.indexed = 1,							\
> +	.channel = 0,							\
> +	.extend_name = _name,						\
> +	.info_mask_separate = _mask,					\
> +	.address = _reg,						\
> +	.info_mask_shared_by_dir = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
This is odd. It defines the sampling frequency.  whilst providing now
such read_raw functionality.  So you will get a sampling frequency attribute
that doesn't work.
> +	.scan_index = -1,						\
There is no buffered support so this isn't needed.

> +}
> +
> +static const struct iio_chan_spec ade7854_channels[] = {
> +	/* Current */
> +	ADE7854_CHANNEL(IIO_CURRENT, PHASEA,
> +			BIT(IIO_CHAN_INFO_SCALE),
Each patch needs to stand on it's own (I might no apply them
all).  This adds the scale attributes, but the support for
them to actually work is in the next patch.

It isn't sensible to break this up into 3 patches and it actually
makes it harder to review (as they only make sense together).

Jonathan

> +			ADE7854_AIGAIN),
> +	ADE7854_CHANNEL(IIO_CURRENT, PHASEB,
> +			BIT(IIO_CHAN_INFO_SCALE),
> +			ADE7854_BIGAIN),
> +	ADE7854_CHANNEL(IIO_CURRENT, PHASEC,
> +			BIT(IIO_CHAN_INFO_SCALE),
> +			ADE7854_CIGAIN),
> +	ADE7854_CHANNEL(IIO_CURRENT, NEUTRAL,
> +			BIT(IIO_CHAN_INFO_SCALE),
> +			ADE7854_NIGAIN),
> +	/* Voltage */
> +	ADE7854_CHANNEL(IIO_VOLTAGE, PHASEA,
> +			BIT(IIO_CHAN_INFO_SCALE),
> +			ADE7854_AVGAIN),
> +	ADE7854_CHANNEL(IIO_VOLTAGE, PHASEB,
> +			BIT(IIO_CHAN_INFO_SCALE),
> +			ADE7854_BVGAIN),
> +	ADE7854_CHANNEL(IIO_VOLTAGE, PHASEC,
> +			BIT(IIO_CHAN_INFO_SCALE),
> +			ADE7854_CVGAIN),
> +};
> +
>  static ssize_t ade7854_read_8bit(struct device *dev,
>  				 struct device_attribute *attr,
>  				 char *buf)

[PATCH 2/3] stagging:iio:meter: Add ade7854_read_raw function

[Rodrigo Siqueira]

This patch adds the ade7854_read_raw() function which is responsible for
handling the read operation for registers: AIGAIN, BIGAIN, CIGAIN,
NIGAIN, AVGAIN, BVGAIN, and CVGAIN. For the sake of simplicity, this
patch only adds basic manipulation for current and voltage channels.
Finally, this patch disables the old approach for reading data.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/meter/ade7854.c | 41 ++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index 2fbb2570ba54..242ecde75900 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -229,31 +229,31 @@ static int ade7854_reset(struct device *dev)
 }
 
 static IIO_DEV_ATTR_AIGAIN(0644,
-		ade7854_read_24bit,
+		NULL,
 		ade7854_write_24bit,
 		ADE7854_AIGAIN);
 static IIO_DEV_ATTR_BIGAIN(0644,
-		ade7854_read_24bit,
+		NULL,
 		ade7854_write_24bit,
 		ADE7854_BIGAIN);
 static IIO_DEV_ATTR_CIGAIN(0644,
-		ade7854_read_24bit,
+		NULL,
 		ade7854_write_24bit,
 		ADE7854_CIGAIN);
 static IIO_DEV_ATTR_NIGAIN(0644,
-		ade7854_read_24bit,
+		NULL,
 		ade7854_write_24bit,
 		ADE7854_NIGAIN);
 static IIO_DEV_ATTR_AVGAIN(0644,
-		ade7854_read_24bit,
+		NULL,
 		ade7854_write_24bit,
 		ADE7854_AVGAIN);
 static IIO_DEV_ATTR_BVGAIN(0644,
-		ade7854_read_24bit,
+		NULL,
 		ade7854_write_24bit,
 		ADE7854_BVGAIN);
 static IIO_DEV_ATTR_CVGAIN(0644,
-		ade7854_read_24bit,
+		NULL,
 		ade7854_write_24bit,
 		ADE7854_CVGAIN);
 static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644,
@@ -471,6 +471,32 @@ static int ade7854_set_irq(struct device *dev, bool enable)
 	return st->write_reg(dev, ADE7854_MASK0, irqen, 32);
 }
 
+static int ade7854_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val,
+			    int *val2,
+			    long mask)
+{
+	struct ade7854_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (mask != IIO_CHAN_INFO_SCALE)
+		return -EINVAL;
+
+	switch (chan->type) {
+	case IIO_CURRENT:
+	case IIO_VOLTAGE:
+		ret = st->read_reg(&indio_dev->dev, chan->address, val, 24);
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT;
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
 static int ade7854_initial_setup(struct iio_dev *indio_dev)
 {
 	int ret;
@@ -572,6 +598,7 @@ static const struct attribute_group ade7854_attribute_group = {
 
 static const struct iio_info ade7854_info = {
 	.attrs = &ade7854_attribute_group,
+	.read_raw = &ade7854_read_raw,
 };
 
 int ade7854_probe(struct iio_dev *indio_dev, struct device *dev)
-- 
2.17.0

Re: [PATCH 2/3] stagging:iio:meter: Add ade7854_read_raw function

[Jonathan Cameron]

On Sat, 21 Apr 2018 08:55:52 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> This patch adds the ade7854_read_raw() function which is responsible for
> handling the read operation for registers: AIGAIN, BIGAIN, CIGAIN,
> NIGAIN, AVGAIN, BVGAIN, and CVGAIN. For the sake of simplicity, this
> patch only adds basic manipulation for current and voltage channels.
> Finally, this patch disables the old approach for reading data.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>

Other than the previous mentioned issue with the way the channels are
defined, this is nearly fine.

See inline.

> ---
>  drivers/staging/iio/meter/ade7854.c | 41 ++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> index 2fbb2570ba54..242ecde75900 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -229,31 +229,31 @@ static int ade7854_reset(struct device *dev)
>  }
>  
>  static IIO_DEV_ATTR_AIGAIN(0644,
> -		ade7854_read_24bit,
> +		NULL,
>  		ade7854_write_24bit,
>  		ADE7854_AIGAIN);
>  static IIO_DEV_ATTR_BIGAIN(0644,
> -		ade7854_read_24bit,
> +		NULL,
>  		ade7854_write_24bit,
>  		ADE7854_BIGAIN);
>  static IIO_DEV_ATTR_CIGAIN(0644,
> -		ade7854_read_24bit,
> +		NULL,
>  		ade7854_write_24bit,
>  		ADE7854_CIGAIN);
>  static IIO_DEV_ATTR_NIGAIN(0644,
> -		ade7854_read_24bit,
> +		NULL,
>  		ade7854_write_24bit,
>  		ADE7854_NIGAIN);
>  static IIO_DEV_ATTR_AVGAIN(0644,
> -		ade7854_read_24bit,
> +		NULL,
>  		ade7854_write_24bit,
>  		ADE7854_AVGAIN);
>  static IIO_DEV_ATTR_BVGAIN(0644,
> -		ade7854_read_24bit,
> +		NULL,
>  		ade7854_write_24bit,
>  		ADE7854_BVGAIN);
>  static IIO_DEV_ATTR_CVGAIN(0644,
> -		ade7854_read_24bit,
> +		NULL,
>  		ade7854_write_24bit,
>  		ADE7854_CVGAIN);
>  static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644,
> @@ -471,6 +471,32 @@ static int ade7854_set_irq(struct device *dev, bool enable)
>  	return st->write_reg(dev, ADE7854_MASK0, irqen, 32);
>  }
>  
> +static int ade7854_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val,
> +			    int *val2,
> +			    long mask)
> +{
> +	struct ade7854_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_SCALE)
> +		return -EINVAL;
Given you specified sampling frequency for the channels in the previous
patch this is a little odd!

> +
> +	switch (chan->type) {
> +	case IIO_CURRENT:
> +	case IIO_VOLTAGE:
> +		ret = st->read_reg(&indio_dev->dev, chan->address, val, 24);
> +		if (ret < 0)
> +			return ret;
> +		return IIO_VAL_INT;
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int ade7854_initial_setup(struct iio_dev *indio_dev)
>  {
>  	int ret;
> @@ -572,6 +598,7 @@ static const struct attribute_group ade7854_attribute_group = {
>  
>  static const struct iio_info ade7854_info = {
>  	.attrs = &ade7854_attribute_group,
> +	.read_raw = &ade7854_read_raw,
>  };
>  
>  int ade7854_probe(struct iio_dev *indio_dev, struct device *dev)

[PATCH 3/3] stagging:iio:meter: Add ade7854_write_raw function

[Rodrigo Siqueira]

This patch adds the ade7854_write_raw() function which is responsible
for handling the write operation for registers: AIGAIN, BIGAIN, CIGAIN,
NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely
removes the old ABI used for handling the registers mentioned above.

Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
---
 drivers/staging/iio/meter/ade7854.c | 60 ++++++++++++-----------------
 1 file changed, 25 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index 242ecde75900..df19c8b4b5d7 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)
 	return st->write_reg(dev, ADE7854_CONFIG, val, 16);
 }
 
-static IIO_DEV_ATTR_AIGAIN(0644,
-		NULL,
-		ade7854_write_24bit,
-		ADE7854_AIGAIN);
-static IIO_DEV_ATTR_BIGAIN(0644,
-		NULL,
-		ade7854_write_24bit,
-		ADE7854_BIGAIN);
-static IIO_DEV_ATTR_CIGAIN(0644,
-		NULL,
-		ade7854_write_24bit,
-		ADE7854_CIGAIN);
-static IIO_DEV_ATTR_NIGAIN(0644,
-		NULL,
-		ade7854_write_24bit,
-		ADE7854_NIGAIN);
-static IIO_DEV_ATTR_AVGAIN(0644,
-		NULL,
-		ade7854_write_24bit,
-		ADE7854_AVGAIN);
-static IIO_DEV_ATTR_BVGAIN(0644,
-		NULL,
-		ade7854_write_24bit,
-		ADE7854_BVGAIN);
-static IIO_DEV_ATTR_CVGAIN(0644,
-		NULL,
-		ade7854_write_24bit,
-		ADE7854_CVGAIN);
 static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644,
 		ade7854_read_24bit,
 		ade7854_write_24bit,
@@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int ade7854_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct ade7854_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if (mask != IIO_CHAN_INFO_SCALE)
+		return -EINVAL;
+
+	switch (chan->type) {
+	case IIO_CURRENT:
+	case IIO_VOLTAGE:
+		ret = st->write_reg(&indio_dev->dev, chan->address, val, 24);
+		if (ret < 0)
+			return ret;
+		return 0;
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
 static int ade7854_initial_setup(struct iio_dev *indio_dev)
 {
 	int ret;
@@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000");
 static IIO_CONST_ATTR(name, "ade7854");
 
 static struct attribute *ade7854_attributes[] = {
-	&iio_dev_attr_aigain.dev_attr.attr,
-	&iio_dev_attr_bigain.dev_attr.attr,
-	&iio_dev_attr_cigain.dev_attr.attr,
-	&iio_dev_attr_nigain.dev_attr.attr,
-	&iio_dev_attr_avgain.dev_attr.attr,
-	&iio_dev_attr_bvgain.dev_attr.attr,
-	&iio_dev_attr_cvgain.dev_attr.attr,
 	&iio_dev_attr_linecyc.dev_attr.attr,
 	&iio_dev_attr_sagcyc.dev_attr.attr,
 	&iio_dev_attr_cfcyc.dev_attr.attr,
@@ -599,6 +588,7 @@ static const struct attribute_group ade7854_attribute_group = {
 static const struct iio_info ade7854_info = {
 	.attrs = &ade7854_attribute_group,
 	.read_raw = &ade7854_read_raw,
+	.write_raw = &ade7854_write_raw,
 };
 
 int ade7854_probe(struct iio_dev *indio_dev, struct device *dev)
-- 
2.17.0

Re: [PATCH 3/3] stagging:iio:meter: Add ade7854_write_raw function

[Jonathan Cameron]

On Sat, 21 Apr 2018 08:56:19 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> This patch adds the ade7854_write_raw() function which is responsible
> for handling the write operation for registers: AIGAIN, BIGAIN, CIGAIN,
> NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely
> removes the old ABI used for handling the registers mentioned above.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
The baby steps approach here is good, but with all 3 patches as one it
would have been a lot easier to follow.

This is almost fine except for the issue I had with how the channels
were defined in the first place.

Also a question about scales inline...

> ---
>  drivers/staging/iio/meter/ade7854.c | 60 ++++++++++++-----------------
>  1 file changed, 25 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> index 242ecde75900..df19c8b4b5d7 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)
>  	return st->write_reg(dev, ADE7854_CONFIG, val, 16);
>  }
>  
> -static IIO_DEV_ATTR_AIGAIN(0644,
> -		NULL,
> -		ade7854_write_24bit,
> -		ADE7854_AIGAIN);
> -static IIO_DEV_ATTR_BIGAIN(0644,
> -		NULL,
> -		ade7854_write_24bit,
> -		ADE7854_BIGAIN);
> -static IIO_DEV_ATTR_CIGAIN(0644,
> -		NULL,
> -		ade7854_write_24bit,
> -		ADE7854_CIGAIN);
> -static IIO_DEV_ATTR_NIGAIN(0644,
> -		NULL,
> -		ade7854_write_24bit,
> -		ADE7854_NIGAIN);
> -static IIO_DEV_ATTR_AVGAIN(0644,
> -		NULL,
> -		ade7854_write_24bit,
> -		ADE7854_AVGAIN);
> -static IIO_DEV_ATTR_BVGAIN(0644,
> -		NULL,
> -		ade7854_write_24bit,
> -		ADE7854_BVGAIN);
> -static IIO_DEV_ATTR_CVGAIN(0644,
> -		NULL,
> -		ade7854_write_24bit,
> -		ADE7854_CVGAIN);
>  static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644,
>  		ade7854_read_24bit,
>  		ade7854_write_24bit,
> @@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static int ade7854_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct ade7854_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_SCALE)
> +		return -EINVAL;
> +
> +	switch (chan->type) {
> +	case IIO_CURRENT:
> +	case IIO_VOLTAGE:
Probably need some range checking to make sure we have a sane value.

Also, I'm curious about units here.

you are using CHAN_INFO_SCALE which would mean this is linked to
the _RAW value (the actual reading of the channel) by _RAW*_SCALE = value in
defined base units for the channel type.  So I'd expect to see some
code here doing a conversion to the internal format for the device.

I wouldn't expect to see a value from userspace written unconverted
into the register.

(I missed this in the previous patch - sorry).

Jonathan

> +		ret = st->write_reg(&indio_dev->dev, chan->address, val, 24);
> +		if (ret < 0)
> +			return ret;
> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static int ade7854_initial_setup(struct iio_dev *indio_dev)
>  {
>  	int ret;
> @@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000");
>  static IIO_CONST_ATTR(name, "ade7854");
>  
>  static struct attribute *ade7854_attributes[] = {
> -	&iio_dev_attr_aigain.dev_attr.attr,
> -	&iio_dev_attr_bigain.dev_attr.attr,
> -	&iio_dev_attr_cigain.dev_attr.attr,
> -	&iio_dev_attr_nigain.dev_attr.attr,
> -	&iio_dev_attr_avgain.dev_attr.attr,
> -	&iio_dev_attr_bvgain.dev_attr.attr,
> -	&iio_dev_attr_cvgain.dev_attr.attr,
>  	&iio_dev_attr_linecyc.dev_attr.attr,
>  	&iio_dev_attr_sagcyc.dev_attr.attr,
>  	&iio_dev_attr_cfcyc.dev_attr.attr,
> @@ -599,6 +588,7 @@ static const struct attribute_group ade7854_attribute_group = {
>  static const struct iio_info ade7854_info = {
>  	.attrs = &ade7854_attribute_group,
>  	.read_raw = &ade7854_read_raw,
> +	.write_raw = &ade7854_write_raw,
>  };
>  
>  int ade7854_probe(struct iio_dev *indio_dev, struct device *dev)

Re: [PATCH 3/3] stagging:iio:meter: Add ade7854_write_raw function

[John Syne]

[-- Attachment #1: Type: text/plain, Size: 5032 bytes --]



Regards,
John





> On Apr 21, 2018, at 10:26 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> 
> On Sat, 21 Apr 2018 08:56:19 -0300
> Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com <mailto:rodrigosiqueiramelo@gmail.com>> wrote:
> 
>> This patch adds the ade7854_write_raw() function which is responsible
>> for handling the write operation for registers: AIGAIN, BIGAIN, CIGAIN,
>> NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely
>> removes the old ABI used for handling the registers mentioned above.
>> 
>> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com <mailto:rodrigosiqueiramelo@gmail.com>>
> The baby steps approach here is good, but with all 3 patches as one it
> would have been a lot easier to follow.
> 
> This is almost fine except for the issue I had with how the channels
> were defined in the first place.
> 
> Also a question about scales inline...
> 
>> ---
>> drivers/staging/iio/meter/ade7854.c | 60 ++++++++++++-----------------
>> 1 file changed, 25 insertions(+), 35 deletions(-)
>> 
>> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
>> index 242ecde75900..df19c8b4b5d7 100644
>> --- a/drivers/staging/iio/meter/ade7854.c
>> +++ b/drivers/staging/iio/meter/ade7854.c
>> @@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)
>> 	return st->write_reg(dev, ADE7854_CONFIG, val, 16);
>> }
>> 
>> -static IIO_DEV_ATTR_AIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_AIGAIN);
>> -static IIO_DEV_ATTR_BIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_BIGAIN);
>> -static IIO_DEV_ATTR_CIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_CIGAIN);
>> -static IIO_DEV_ATTR_NIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_NIGAIN);
>> -static IIO_DEV_ATTR_AVGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_AVGAIN);
>> -static IIO_DEV_ATTR_BVGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_BVGAIN);
>> -static IIO_DEV_ATTR_CVGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_CVGAIN);
>> static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644,
>> 		ade7854_read_24bit,
>> 		ade7854_write_24bit,
>> @@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev *indio_dev,
>> 	return -EINVAL;
>> }
>> 
>> +static int ade7854_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int val, int val2, long mask)
>> +{
>> +	struct ade7854_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	if (mask != IIO_CHAN_INFO_SCALE)
>> +		return -EINVAL;
>> +
>> +	switch (chan->type) {
>> +	case IIO_CURRENT:
>> +	case IIO_VOLTAGE:
> Probably need some range checking to make sure we have a sane value.
> 
> Also, I'm curious about units here.
> 
> you are using CHAN_INFO_SCALE which would mean this is linked to
> the _RAW value (the actual reading of the channel) by _RAW*_SCALE = value in
> defined base units for the channel type.  So I'd expect to see some
> code here doing a conversion to the internal format for the device.
These should be IIO_CHAN_INFO_HARDWAREGAIN as they are used to correct 
any imperfections of the sensors. A user space application calculates the error and
based on a formula determines the correct register value. 

Meter values such as Voltage, Current, Power, Energy use a value/LSB method, 
which is determined while applying a known voltage and current. After that, any
parameter is multiplied by the respective value/LSB to get a real world value. 

Regards,
John
> 
> I wouldn't expect to see a value from userspace written unconverted
> into the register.
> 
> (I missed this in the previous patch - sorry).
> 
> Jonathan
> 
>> +		ret = st->write_reg(&indio_dev->dev, chan->address, val, 24);
>> +		if (ret < 0)
>> +			return ret;
>> +		return 0;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> static int ade7854_initial_setup(struct iio_dev *indio_dev)
>> {
>> 	int ret;
>> @@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000");
>> static IIO_CONST_ATTR(name, "ade7854");
>> 
>> static struct attribute *ade7854_attributes[] = {
>> -	&iio_dev_attr_aigain.dev_attr.attr,
>> -	&iio_dev_attr_bigain.dev_attr.attr,
>> -	&iio_dev_attr_cigain.dev_attr.attr,
>> -	&iio_dev_attr_nigain.dev_attr.attr,
>> -	&iio_dev_attr_avgain.dev_attr.attr,
>> -	&iio_dev_attr_bvgain.dev_attr.attr,
>> -	&iio_dev_attr_cvgain.dev_attr.attr,
>> 	&iio_dev_attr_linecyc.dev_attr.attr,
>> 	&iio_dev_attr_sagcyc.dev_attr.attr,
>> 	&iio_dev_attr_cfcyc.dev_attr.attr,
>> @@ -599,6 +588,7 @@ static const struct attribute_group ade7854_attribute_group = {
>> static const struct iio_info ade7854_info = {
>> 	.attrs = &ade7854_attribute_group,
>> 	.read_raw = &ade7854_read_raw,
>> +	.write_raw = &ade7854_write_raw,
>> };
>> 
>> int ade7854_probe(struct iio_dev *indio_dev, struct device *dev)


[-- Attachment #2: Type: text/html, Size: 30753 bytes --]

Re: [PATCH 3/3] stagging:iio:meter: Add ade7854_write_raw function

[John Syne]

> On Apr 21, 2018, at 10:26 AM, Jonathan Cameron <jic23@kernel.org> =
wrote:
>=20
> On Sat, 21 Apr 2018 08:56:19 -0300
> Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
>=20
>> This patch adds the ade7854_write_raw() function which is responsible
>> for handling the write operation for registers: AIGAIN, BIGAIN, =
CIGAIN,
>> NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely
>> removes the old ABI used for handling the registers mentioned above.
>>=20
>> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> The baby steps approach here is good, but with all 3 patches as one it
> would have been a lot easier to follow.
>=20
> This is almost fine except for the issue I had with how the channels
> were defined in the first place.
>=20
> Also a question about scales inline...
>=20
>> ---
>> drivers/staging/iio/meter/ade7854.c | 60 =
++++++++++++-----------------
>> 1 file changed, 25 insertions(+), 35 deletions(-)
>>=20
>> diff --git a/drivers/staging/iio/meter/ade7854.c =
b/drivers/staging/iio/meter/ade7854.c
>> index 242ecde75900..df19c8b4b5d7 100644
>> --- a/drivers/staging/iio/meter/ade7854.c
>> +++ b/drivers/staging/iio/meter/ade7854.c
>> @@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)
>> 	return st->write_reg(dev, ADE7854_CONFIG, val, 16);
>> }
>>=20
>> -static IIO_DEV_ATTR_AIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_AIGAIN);
>> -static IIO_DEV_ATTR_BIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_BIGAIN);
>> -static IIO_DEV_ATTR_CIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_CIGAIN);
>> -static IIO_DEV_ATTR_NIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_NIGAIN);
>> -static IIO_DEV_ATTR_AVGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_AVGAIN);
>> -static IIO_DEV_ATTR_BVGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_BVGAIN);
>> -static IIO_DEV_ATTR_CVGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_CVGAIN);
>> static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644,
>> 		ade7854_read_24bit,
>> 		ade7854_write_24bit,
>> @@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev =
*indio_dev,
>> 	return -EINVAL;
>> }
>>=20
>> +static int ade7854_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int val, int val2, long mask)
>> +{
>> +	struct ade7854_state *st =3D iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	if (mask !=3D IIO_CHAN_INFO_SCALE)
>> +		return -EINVAL;
>> +
>> +	switch (chan->type) {
>> +	case IIO_CURRENT:
>> +	case IIO_VOLTAGE:
> Probably need some range checking to make sure we have a sane value.
>=20
> Also, I'm curious about units here.
>=20
> you are using CHAN_INFO_SCALE which would mean this is linked to
> the _RAW value (the actual reading of the channel) by _RAW*_SCALE =3D =
value in
> defined base units for the channel type.  So I'd expect to see some
> code here doing a conversion to the internal format for the device.
These should be IIO_CHAN_INFO_HARDWAREGAIN as they are used to correct=20=

any imperfections of the sensors. A user space application calculates =
the error and
based on a formula determines the correct register value.=20

Meter values such as Voltage, Current, Power, Energy use a value/LSB =
method,=20
which is determined while applying a known voltage and current. After =
that, any
parameter is multiplied by the respective value/LSB to get a real world =
value.=20

Regards,
John
>=20
> I wouldn't expect to see a value from userspace written unconverted
> into the register.
>=20
> (I missed this in the previous patch - sorry).
>=20
> Jonathan
>=20
>> +		ret =3D st->write_reg(&indio_dev->dev, chan->address, =
val, 24);
>> +		if (ret < 0)
>> +			return ret;
>> +		return 0;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> static int ade7854_initial_setup(struct iio_dev *indio_dev)
>> {
>> 	int ret;
>> @@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000");
>> static IIO_CONST_ATTR(name, "ade7854");
>>=20
>> static struct attribute *ade7854_attributes[] =3D {
>> -	&iio_dev_attr_aigain.dev_attr.attr,
>> -	&iio_dev_attr_bigain.dev_attr.attr,
>> -	&iio_dev_attr_cigain.dev_attr.attr,
>> -	&iio_dev_attr_nigain.dev_attr.attr,
>> -	&iio_dev_attr_avgain.dev_attr.attr,
>> -	&iio_dev_attr_bvgain.dev_attr.attr,
>> -	&iio_dev_attr_cvgain.dev_attr.attr,
>> 	&iio_dev_attr_linecyc.dev_attr.attr,
>> 	&iio_dev_attr_sagcyc.dev_attr.attr,
>> 	&iio_dev_attr_cfcyc.dev_attr.attr,
>> @@ -599,6 +588,7 @@ static const struct attribute_group =
ade7854_attribute_group =3D {
>> static const struct iio_info ade7854_info =3D {
>> 	.attrs =3D &ade7854_attribute_group,
>> 	.read_raw =3D &ade7854_read_raw,
>> +	.write_raw =3D &ade7854_write_raw,
>> };
>>=20
>> int ade7854_probe(struct iio_dev *indio_dev, struct device *dev)

Re: [PATCH 3/3] stagging:iio:meter: Add ade7854_write_raw function

[John Syne]

[-- Attachment #1: Type: text/plain, Size: 78 bytes --]

Attached is a spreadsheet which should help explain the calibration process.


[-- Attachment #2: ADE7878_calibration.xlsx --]
[-- Type: application/vnd.openxmlformats-officedocument.spreadsheetml.sheet, Size: 51854 bytes --]

[-- Attachment #3: Type: text/plain, Size: 4601 bytes --]




Here is the calibration guide:
http://www.analog.com/media/en/technical-documentation/application-notes/AN-1076.pdf



Regards,
John





> On Apr 21, 2018, at 10:26 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> 
> On Sat, 21 Apr 2018 08:56:19 -0300
> Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
> 
>> This patch adds the ade7854_write_raw() function which is responsible
>> for handling the write operation for registers: AIGAIN, BIGAIN, CIGAIN,
>> NIGAIN, AVGAIN, BVGAIN, and CVGAIN. Finally, this patch completely
>> removes the old ABI used for handling the registers mentioned above.
>> 
>> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> The baby steps approach here is good, but with all 3 patches as one it
> would have been a lot easier to follow.
> 
> This is almost fine except for the issue I had with how the channels
> were defined in the first place.
> 
> Also a question about scales inline...
> 
>> ---
>> drivers/staging/iio/meter/ade7854.c | 60 ++++++++++++-----------------
>> 1 file changed, 25 insertions(+), 35 deletions(-)
>> 
>> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
>> index 242ecde75900..df19c8b4b5d7 100644
>> --- a/drivers/staging/iio/meter/ade7854.c
>> +++ b/drivers/staging/iio/meter/ade7854.c
>> @@ -228,34 +228,6 @@ static int ade7854_reset(struct device *dev)
>> 	return st->write_reg(dev, ADE7854_CONFIG, val, 16);
>> }
>> 
>> -static IIO_DEV_ATTR_AIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_AIGAIN);
>> -static IIO_DEV_ATTR_BIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_BIGAIN);
>> -static IIO_DEV_ATTR_CIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_CIGAIN);
>> -static IIO_DEV_ATTR_NIGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_NIGAIN);
>> -static IIO_DEV_ATTR_AVGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_AVGAIN);
>> -static IIO_DEV_ATTR_BVGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_BVGAIN);
>> -static IIO_DEV_ATTR_CVGAIN(0644,
>> -		NULL,
>> -		ade7854_write_24bit,
>> -		ADE7854_CVGAIN);
>> static IIO_DEV_ATTR_APPARENT_POWER_A_GAIN(0644,
>> 		ade7854_read_24bit,
>> 		ade7854_write_24bit,
>> @@ -497,6 +469,30 @@ static int ade7854_read_raw(struct iio_dev *indio_dev,
>> 	return -EINVAL;
>> }
>> 
>> +static int ade7854_write_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int val, int val2, long mask)
>> +{
>> +	struct ade7854_state *st = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	if (mask != IIO_CHAN_INFO_SCALE)
>> +		return -EINVAL;
>> +
>> +	switch (chan->type) {
>> +	case IIO_CURRENT:
>> +	case IIO_VOLTAGE:
> Probably need some range checking to make sure we have a sane value.
> 
> Also, I'm curious about units here.
> 
> you are using CHAN_INFO_SCALE which would mean this is linked to
> the _RAW value (the actual reading of the channel) by _RAW*_SCALE = value in
> defined base units for the channel type.  So I'd expect to see some
> code here doing a conversion to the internal format for the device.
> 
> I wouldn't expect to see a value from userspace written unconverted
> into the register.
> 
> (I missed this in the previous patch - sorry).
> 
> Jonathan
> 
>> +		ret = st->write_reg(&indio_dev->dev, chan->address, val, 24);
>> +		if (ret < 0)
>> +			return ret;
>> +		return 0;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> static int ade7854_initial_setup(struct iio_dev *indio_dev)
>> {
>> 	int ret;
>> @@ -521,13 +517,6 @@ static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("8000");
>> static IIO_CONST_ATTR(name, "ade7854");
>> 
>> static struct attribute *ade7854_attributes[] = {
>> -	&iio_dev_attr_aigain.dev_attr.attr,
>> -	&iio_dev_attr_bigain.dev_attr.attr,
>> -	&iio_dev_attr_cigain.dev_attr.attr,
>> -	&iio_dev_attr_nigain.dev_attr.attr,
>> -	&iio_dev_attr_avgain.dev_attr.attr,
>> -	&iio_dev_attr_bvgain.dev_attr.attr,
>> -	&iio_dev_attr_cvgain.dev_attr.attr,
>> 	&iio_dev_attr_linecyc.dev_attr.attr,
>> 	&iio_dev_attr_sagcyc.dev_attr.attr,
>> 	&iio_dev_attr_cfcyc.dev_attr.attr,
>> @@ -599,6 +588,7 @@ static const struct attribute_group ade7854_attribute_group = {
>> static const struct iio_info ade7854_info = {
>> 	.attrs = &ade7854_attribute_group,
>> 	.read_raw = &ade7854_read_raw,
>> +	.write_raw = &ade7854_write_raw,
>> };
>> 
>> int ade7854_probe(struct iio_dev *indio_dev, struct device *dev)

Re: [PATCH 0/3] stagging:iio:meter: Add essential IIO API structures for ADE7854

[Jonathan Cameron]

On Sat, 21 Apr 2018 08:54:45 -0300
Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:

> This patchset aims to update ADE7854 by adding the required IIO API
> components. The first patch adds the iio_chan_spec for handling seven
> different registers (all of them with a similar behavior). The second
> patch appends the read_raw function defined by the IIO API. Finally, the
> third patch adds the write_raw function and remove the attributes used
> for handling the seven registers. This patchset has the contribution of
> John Syne, which was responsible for mapping the correct ABI name per
> element in the ADE7854; additionally, John provided codes that helped to
> shape these patches.
> 
> Rodrigo Siqueira (3):
>   stagging:iio:meter: Add iio_chan_spec
>   stagging:iio:meter: Add ade7854_read_raw function
>   stagging:iio:meter: Add ade7854_write_raw function
> 
>  drivers/staging/iio/meter/ade7854.c | 129 ++++++++++++++++++++--------
>  1 file changed, 94 insertions(+), 35 deletions(-)
> 
Hi Rodrigo,

Please don't do it like this.   The original discussion with John
involved the addition of an extra chunk of core logic so we would have
the ability to specify the channel without using extended_name.

Extended name is not intended to differentiate between channels (it
isn't in general enough to do so - though it works in this little
corner case of sysfs attributes like you have here).

So the plan is to add another field to the iio_chan_spec structure
to allow for the complexity we need.  See the long discussions
over how we represent the myriad of channels in here.

If you need some pointers, feel free to ask but it may take
me a little while to put together a full description of what needs
doing.

I'll put a few more direct comments in the individual patches.

Thanks,

Jonathan

Re: [PATCH 0/3] stagging:iio:meter: Add essential IIO API structures for ADE7854

[Rodrigo Siqueira]

Hi Jonathan,

Thanks for all the feedbacks and sorry for some basic mistakes, I will
work on the second version based on all your comments.

Best Regards
Rodrigo Siqueira

On 04/21, Jonathan Cameron wrote:
> On Sat, 21 Apr 2018 08:54:45 -0300
> Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote:
> 
> > This patchset aims to update ADE7854 by adding the required IIO API
> > components. The first patch adds the iio_chan_spec for handling seven
> > different registers (all of them with a similar behavior). The second
> > patch appends the read_raw function defined by the IIO API. Finally, the
> > third patch adds the write_raw function and remove the attributes used
> > for handling the seven registers. This patchset has the contribution of
> > John Syne, which was responsible for mapping the correct ABI name per
> > element in the ADE7854; additionally, John provided codes that helped to
> > shape these patches.
> > 
> > Rodrigo Siqueira (3):
> >   stagging:iio:meter: Add iio_chan_spec
> >   stagging:iio:meter: Add ade7854_read_raw function
> >   stagging:iio:meter: Add ade7854_write_raw function
> > 
> >  drivers/staging/iio/meter/ade7854.c | 129 ++++++++++++++++++++--------
> >  1 file changed, 94 insertions(+), 35 deletions(-)
> > 
> Hi Rodrigo,
> 
> Please don't do it like this.   The original discussion with John
> involved the addition of an extra chunk of core logic so we would have
> the ability to specify the channel without using extended_name.
> 
> Extended name is not intended to differentiate between channels (it
> isn't in general enough to do so - though it works in this little
> corner case of sysfs attributes like you have here).
> 
> So the plan is to add another field to the iio_chan_spec structure
> to allow for the complexity we need.  See the long discussions
> over how we represent the myriad of channels in here.
> 
> If you need some pointers, feel free to ask but it may take
> me a little while to put together a full description of what needs
> doing.
> 
> I'll put a few more direct comments in the individual patches.
> 
> Thanks,
> 
> Jonathan
>